Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove unnecessary this overwrite in stack context manager #873

Merged
merged 2 commits into from
Mar 19, 2020

Conversation

dyladan
Copy link
Member

@dyladan dyladan commented Mar 18, 2020

Stack scope manager was calling fn.apply(context, args) which causes the this in fn to be set to context rather than what it would have been normally. It appears not to affect the working of the Context Manager

@@ -112,7 +112,7 @@ export class StackContextManager implements ContextManager {
this._currentContext = context || Context.ROOT_CONTEXT;

try {
return fn.apply(context);
return fn();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@obecny would like to get your reviews on this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm it was a bit tricky from what I remember, but if the all tests are working fine, then I guess it should be fine too ?. Can you check the the examples replacing the zone scope manager with the stack if nothing is missing ?.

@dyladan dyladan changed the title Do not mangle in stack context manager Remove unnecessary this overwrite in stack context manager Mar 19, 2020
Copy link
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just one comment

@@ -112,7 +112,7 @@ export class StackContextManager implements ContextManager {
this._currentContext = context || Context.ROOT_CONTEXT;

try {
return fn.apply(context);
return fn();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm it was a bit tricky from what I remember, but if the all tests are working fine, then I guess it should be fine too ?. Can you check the the examples replacing the zone scope manager with the stack if nothing is missing ?.

@dyladan dyladan added the bug Something isn't working label Mar 19, 2020
@dyladan
Copy link
Member Author

dyladan commented Mar 19, 2020

@obecny the tracer web example is failing for unrelated reasons. The window.addEventListener fails. It fails in the same way with and without the changes. Because the tests are passing and it appears to be working with my local testing, i'm going to merge this.

@dyladan dyladan merged commit aa3902f into open-telemetry:master Mar 19, 2020
@dyladan dyladan deleted the stack-this branch March 19, 2020 23:28
pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this pull request Dec 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants